Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: genesis auth account format #517

Merged
merged 12 commits into from
May 11, 2022

Conversation

dudong2
Copy link
Contributor

@dudong2 dudong2 commented Apr 27, 2022

Description

I restored the part(MarshalX, UnmarshalX) that was removed due to a build failure while bumping up to [email protected].

wrong format

{
  "@type": "/lbm.auth.v1.BaseAccount",
  "account_number": "0",
  "address": "link146asaycmtydq45kxc8evntqfgepagygelel00h",
  "ed25519_pub_key": null,
  "multisig_pub_key": null,
  "secp256k1_pub_key": {
      "key": "AgT2QPS4Eu6M+cfHeba+3tumsM/hNEBGdM7nRojSZRjF"
  },
  "secp256r1_pub_key": null,
  "sequence": "1"
},

right format

{
  "@type": "/lbm.auth.v1.BaseAccount",
  "account_number": 0,
  "address": "link146asaycmtydq45kxc8evntqfgepagygelel00h",
  "pub_key": {
      "key": "eyJrZXkiOiJBZ1QyUVBTNEV1Nk0rY2ZIZWJhKzN0dW1zTS9oTkVCR2RNN25Sb2pTWlJqRiJ9",
      "type": 1
  },
  "sequence": "1"
},

closes: #515

Motivation and context

How has this been tested?

Screenshots (if appropriate):

Checklist:

  • I followed the contributing guidelines and code of conduct.
  • I have added a relevant changelog to CHANGELOG.md
  • I have added tests to cover my changes.
  • I have updated the documentation accordingly.
  • I have updated API documentation client/docs/swagger-ui/swagger.yaml

@dudong2 dudong2 added the A: bug Something isn't working label Apr 27, 2022
@dudong2 dudong2 self-assigned this Apr 27, 2022
@codecov
Copy link

codecov bot commented Apr 27, 2022

Codecov Report

Merging #517 (ea11c89) into main (396a1fe) will increase coverage by 0.09%.
The diff coverage is 66.30%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #517      +/-   ##
==========================================
+ Coverage   56.75%   56.85%   +0.09%     
==========================================
  Files         777      777              
  Lines       83389    83633     +244     
==========================================
+ Hits        47331    47547     +216     
- Misses      33004    33007       +3     
- Partials     3054     3079      +25     
Impacted Files Coverage Δ
x/bank/keeper/keeper.go 68.97% <0.00%> (-0.67%) ⬇️
x/bank/keeper/send.go 75.39% <12.50%> (-2.15%) ⬇️
x/auth/types/account.go 61.70% <52.03%> (+1.08%) ⬆️
x/auth/vesting/types/vesting_account.go 88.32% <82.60%> (-1.55%) ⬇️
x/auth/keeper/keeper.go 51.00% <100.00%> (-0.49%) ⬇️
x/wasm/keeper/query_plugins.go 73.26% <0.00%> (-0.51%) ⬇️
x/wasm/types/proposal.go 81.14% <0.00%> (ø)
x/wasm/keeper/legacy_querier.go 65.51% <0.00%> (ø)
x/consortium/keeper/querier.go
... and 14 more

@dudong2 dudong2 requested review from 0Tech, loin3 and zemyblue April 27, 2022 23:42
@@ -46,6 +46,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* (x/wasm) [\#453](https://github.com/line/lbm-sdk/pull/453) modify wasm grpc query api path
* (client) [\#476](https://github.com/line/lbm-sdk/pull/476) change the default value of the client output format in the config
* (server/grpc) [\#516](https://github.com/line/lbm-sdk/pull/516) restore build norace flag
* (genesis) [\#517](https://github.com/line/lbm-sdk/pull/517) fix genesis auth account format(cosmos-sdk style -> lbm-sdk style)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lbm-sdk -> cosmos-sdk style ?

@@ -319,7 +319,7 @@ func TestInstantiate(t *testing.T) {

gasAfter := ctx.GasMeter().GasConsumed()
if types.EnableGasVerification {
require.Equal(t, uint64(0x142fa), gasAfter-gasBefore)
require.Equal(t, uint64(0x1402a), gasAfter-gasBefore)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the fee reduced?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the cause is the decrease in the size of msg as pubkey is distinguished by type.

@zemyblue
Copy link
Member

zemyblue commented May 3, 2022

The unittest of vestingAccount's marchaling and unmarshaling functions were not existed. Please add it and increase the coverage.

@dudong2 dudong2 requested a review from brew0722 May 9, 2022 01:55
Copy link
Member

@zemyblue zemyblue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Collaborator

@0Tech 0Tech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but some improvements.

x/auth/types/account.go Outdated Show resolved Hide resolved
x/auth/types/account_test.go Outdated Show resolved Hide resolved
x/auth/types/account_test.go Outdated Show resolved Hide resolved
x/auth/vesting/types/vesting_account_test.go Outdated Show resolved Hide resolved
x/auth/vesting/types/vesting_account_test.go Outdated Show resolved Hide resolved
x/auth/vesting/types/vesting_account_test.go Outdated Show resolved Hide resolved
x/auth/vesting/types/vesting_account_test.go Outdated Show resolved Hide resolved
x/auth/vesting/types/vesting_account_test.go Outdated Show resolved Hide resolved
x/auth/vesting/types/vesting_account_test.go Outdated Show resolved Hide resolved
x/auth/vesting/types/vesting_account_test.go Outdated Show resolved Hide resolved
@dudong2 dudong2 merged commit 9b70a6d into Finschia:main May 11, 2022
@dudong2 dudong2 deleted the dudong2/fix/genesis-account branch May 11, 2022 04:49
brew0722 pushed a commit to brew0722/lbm-sdk that referenced this pull request May 11, 2022
* main:
  fix: genesis auth account format (Finschia#517)
  Remove reviewers on dependabot.yml
  updated ostracon to v1.0.5; `unsafe-reset-all` command has been moved to the `ostracon` sub-command. (Finschia#536)
  build(deps): bump docker/build-push-action from 2 to 3 (Finschia#532)
  build(deps): bump docker/metadata-action from 3 to 4 (Finschia#530)
  build(deps): bump docker/login-action from 1 to 2 (Finschia#531)
  build(deps): bump docker/setup-buildx-action from 1 to 2 (Finschia#529)
  chore(deps): bump github.com/VictoriaMetrics/fastcache from 1.9.0 to 1.10.0 (Finschia#499)
  build(deps): bump google.golang.org/protobuf from 1.27.1 to 1.28.0 (Finschia#474)
  chore(deps): bump github.com/prometheus/common from 0.32.1 to 0.34.0 (Finschia#510)
  chore(deps): bump github.com/coinbase/rosetta-sdk-go from 0.7.0 to 0.7.9 (Finschia#520)
  build(deps): bump github.com/spf13/cobra from 1.3.0 to 1.4.0 (Finschia#458)
  build(deps): bump github.com/confio/ics23/go from 0.6.6 to 0.7.0 (Finschia#448)
  build(deps): bump github.com/stretchr/testify from 1.7.0 to 1.7.1 (Finschia#465)
  build(deps): bump github.com/armon/go-metrics from 0.3.10 to 0.3.11 (Finschia#522)
  build(deps): bump github.com/magiconair/properties from 1.8.5 to 1.8.6 (Finschia#445)
  ci: change automerge conditions of Mergify (Finschia#523)
@zemyblue zemyblue mentioned this pull request Oct 27, 2022
5 tasks
@zemyblue zemyblue mentioned this pull request Nov 28, 2022
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pubkey info of app_state.auth.accounts is separated in genesis
3 participants